Some more cleanups in Assembly/Binder/Loader area #59590

VSadov · 2021-09-24T22:46:19Z

More tidy-up and removal of no longer necessary code.

moved default binder allocation to AppDomain::Create
As discussed in the previous PR.
folded PEFile into PEAssembly. We only have PEAssembly now.
All the PE files we deal with are assemblies and PEFile is unnecessary abstraction that was created to support modules, resource-only files, IStream things provided by SQL host, etc... None of that exists any more.
Renames and cleanups related to the PEAssembly change.
Unlike PEFile, PEAssembly is always an assembly, never a native Resource PE, always has metadata, has MDImport right after construction, its PEImage is opened at construction and also has metadata, and so on...
Many checks and conditions could be simplified.
AppDomain no longer has a scenario when a hosted assembly is updated after publishing, so the code supporting such scenario can be removed.
Some cleanup of PEImage - moved instance filed declarations together, sorted related public/private members, removed unused/redundant stuff.
I.E. GetLayout is always called with LAYOUT_CREATEIFNEEDED, so the flag is unnecessary, and similar changes. Overall functionality is the same as before.

ghost · 2021-09-24T22:46:25Z

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

In-progress. Running tests.

One of the goals is to combine PEFile and PEAssembly.
All the PE files we deal with are assemblies and PEFile is unnecessary abstraction that was created to support modules, resource-only files, IStream things provided by SQL host, etc... None of that exists any more.

Later we should look at how much sharing is between PEAssembly, PEImage and BINDER_SPACE::Assembly.

Author:	VSadov
Assignees:	-
Labels:	`area-AssemblyLoader-coreclr`
Milestone:	-

… in CoreClr

…cenario.

elinor-fung

Looked through the headers so far.
I'm a pretty slow reviewer over here 😬

src/coreclr/vm/ceeload.cpp

src/coreclr/vm/commodule.cpp

src/coreclr/vm/peassembly.h

src/coreclr/vm/peimage.h

elinor-fung · 2021-10-05T03:21:48Z

src/coreclr/vm/peassembly.cpp

+        // TODO: we used to return "m_pMDImport_UseAccessor" here, - a field that was never initialized to anything.
+        //       we should verify whether this codepath is actually reachable.
+        // return (TADDR)m_pMDImport_UseAccessor;


@dotnet/dotnet-diag?

This code is weird, but should be left as it was.

We rename PEFile::m_pMDImport to m_pMDImport_UseAccessor when compiling for the DAC, because we want it to be a compile error if anybody tries to use m_pMDImport in the DAC, since m_pMDImport is the TADDR (debuggee side address) and not a PTR_* that knows how to magically read the value from the debuggee.

However, this one method we actually want to read the TADDR value , do we use m_pMDImport_UseAccessor directly in the DAC code. The code in DacDbiInterfaceImpl::GetPEFileMDInternalRW calls GetMDInternalRWAddress, which will read the right value since m_pMDImport_UseAccessor has the same type and offset as m_pMDImport

Thanks @davmason!! I think I got it. I also think this deserves a better comment. It is fairly hard to figure where and how this field is used by just searching around.
I will try adding your comment there as well in some form - at least it would hint that there is more going on than a textual search might reveal.

src/coreclr/vm/peassembly.h

Co-authored-by: Elinor Fung <elfung@microsoft.com>

VSadov · 2021-10-05T17:28:21Z

@elinor-fung @davmason - I think I have addressed all the comments. Please take a look that all makes sense and I did not miss something.

vitek-karas · 2021-10-06T12:47:48Z

src/coreclr/vm/peassembly.h

+    // be the parent assembly for dynamic or memory assemblies
+    const SString& GetEffectivePath();
+
+    // Codebase is the fusion codebase or path for the assembly.  It is in URL format.


Another reference to "fusion" which should be removed.

Quick search finds 48 references to "fusion" in the comments. It is not always clear how to re-word them, and the old context may still be useful while refactoring.
I think it may be worth to revisit "fusion" comments a bit later. Fortunately it is such an easy thing to find.

davmason

The diagnostics side stuff looks good to me, I didn't take a look at the broader code changes though

src/coreclr/vm/encee.cpp

elinor-fung · 2021-10-05T17:24:48Z

src/coreclr/vm/proftoeeinterfaceimpl.cpp

@@ -4299,7 +4286,7 @@ HRESULT ProfToEEInterfaceImpl::GetILFunctionBody(ModuleID    moduleId,
        // Yay!
        MODE_ANY;

-        // PEFile::CheckLoaded & Module::GetDynamicIL both take a lock
+        // PEAssembly::HasLoadedPEImage & Module::GetDynamicIL both take a lock


Is this still true for PEAssembly::HasLoadedPEImage?

Not true for PEAssembly::HasLoadedPEImage, but still true for Module::GetDynamicIL

src/coreclr/vm/assembly.cpp

src/coreclr/vm/peimage.cpp

src/coreclr/debug/daccess/daccess.cpp

src/coreclr/vm/peassembly.cpp

elinor-fung · 2021-10-07T01:16:45Z

src/coreclr/vm/peassembly.cpp

 {
    WRAPPER_NO_CONTRACT;
    SUPPORTS_DAC;

-    // sizeof(PEFile) == 0xb8
+    // sizeof(PEAssembly) == 0xb8


Is this comment about the size still true?

Most likely not and I do not know what it means. I do not think we have dependency on the size of the instance and I am not sure the size was correct before - was it for x86 or x64, etc?
There is no PEFile anyways. I will remove the comment.

Co-authored-by: Elinor Fung <elfung@microsoft.com>

VSadov · 2021-10-11T20:33:44Z

@elinor-fung - anything else that could be addressed on this PR?

elinor-fung

Sorry - completely missed coming back to this. Looks good to me.

VSadov · 2021-10-11T20:52:13Z

Thanks!! They were good suggestions.

dotnet-issue-labeler bot added the area-AssemblyLoader-coreclr label Sep 24, 2021

VSadov force-pushed the aloader05 branch from 55b71a0 to 3b4b4d6 Compare September 25, 2021 02:46

runfoapp bot mentioned this pull request Sep 28, 2021

System.Text.RegularExpressions.Tests.RegexMatchTests.Match_ExcessPrefix Failing in CI #59541

Closed

VSadov force-pushed the aloader05 branch from 936bb45 to b7f1549 Compare September 29, 2021 01:20

VSadov added 13 commits September 30, 2021 18:32

moved default binder allocation to AppDomain::Create

749bb9f

allowNativeSkip parameter is unused.

acbfbae

Removed PEFile::IsResource and PEFile::IsIStream - these do not exist…

5ce2858

… in CoreClr

make GCC happy

20b54a8

folded impl

f7c04ba

Folded PEFile into PEAssembly

e87a04f

renamed pefile files

a349432

reorder PEAssembly members

ecb7135

removed meaningless LAYOUT_CREATEIFNEEDED - we always require that

3f37d84

r2r bundled image should Open via cache - no reasons not to.

51c3b64

Some cleanup in PEImage

2500c72

PEAssembly needs only one reference to PEImage

dc1a35a

a few tweaks

48e68e5

VSadov force-pushed the aloader05 branch from 551d927 to 48e68e5 Compare October 1, 2021 04:59

VSadov added 5 commits October 1, 2021 11:48

some more

c4cb223

We never update published hosted assemblies, no need to support the s…

2b2b7d9

…cenario.

Done with PEAssembly for now

8bf2125

couple fixes, mostly in comments afected by renames

835df34

more cleanups for PEImage

45057d0

VSadov marked this pull request as ready for review October 4, 2021 16:44

VSadov requested review from elinor-fung and vitek-karas October 4, 2021 16:44

elinor-fung reviewed Oct 5, 2021

View reviewed changes

VSadov and others added 2 commits October 5, 2021 09:17

PR review suggestion

173797f

Co-authored-by: Elinor Fung <elfung@microsoft.com>

PR review feedback

58abf40

this line should be deleted, not commented.

9286a09

vitek-karas reviewed Oct 6, 2021

View reviewed changes

davmason approved these changes Oct 6, 2021

View reviewed changes

elinor-fung reviewed Oct 7, 2021

View reviewed changes

VSadov and others added 2 commits October 7, 2021 13:39

Apply suggestions from code review

512319b

Co-authored-by: Elinor Fung <elfung@microsoft.com>

More PR feedback

efdc145

VSadov requested a review from elinor-fung October 8, 2021 16:52

elinor-fung approved these changes Oct 11, 2021

View reviewed changes

VSadov merged commit 7088332 into dotnet:main Oct 11, 2021

VSadov deleted the aloader05 branch October 11, 2021 20:52

jkoritzinsky mentioned this pull request Oct 11, 2021

Merge feature/use-dllimport-generator to main #59579

Merged

AaronRobinsonMSFT mentioned this pull request Oct 13, 2021

DAC crashes WinDBGX on DBG/CHK builds #60321

Closed

hoyosjs mentioned this pull request Oct 14, 2021

Don't error out from GetOrCreateLayout in the DAC if no layout creation is needed #60401

Merged

ghost locked as resolved and limited conversation to collaborators Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some more cleanups in Assembly/Binder/Loader area #59590

Some more cleanups in Assembly/Binder/Loader area #59590

VSadov commented Sep 24, 2021 •

edited

Loading

ghost commented Sep 24, 2021

elinor-fung left a comment

elinor-fung Oct 5, 2021

davmason Oct 5, 2021

VSadov Oct 5, 2021 •

edited

Loading

VSadov commented Oct 5, 2021

vitek-karas Oct 6, 2021

VSadov Oct 6, 2021

davmason left a comment

elinor-fung Oct 5, 2021

VSadov Oct 7, 2021

elinor-fung Oct 7, 2021

VSadov Oct 7, 2021

VSadov commented Oct 11, 2021

elinor-fung left a comment

VSadov commented Oct 11, 2021

Some more cleanups in Assembly/Binder/Loader area #59590

Some more cleanups in Assembly/Binder/Loader area #59590

Conversation

VSadov commented Sep 24, 2021 • edited Loading

ghost commented Sep 24, 2021

elinor-fung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

VSadov commented Oct 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davmason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov commented Oct 11, 2021

elinor-fung left a comment

Choose a reason for hiding this comment

VSadov commented Oct 11, 2021

VSadov commented Sep 24, 2021 •

edited

Loading

VSadov Oct 5, 2021 •

edited

Loading